Skip to content

[analyzer][NFCi] Pass if bind is to a Decl or not to checkBind #152137

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Aug 8, 2025

Conversation

gamesh411
Copy link
Contributor

Binding a value to location can happen when a new value is created or when and existing value is updated. This modification exposes whether the value binding happens at a declaration.
This helps simplify the hacky logic of the BindToImmutable checker.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:static analyzer labels Aug 5, 2025
@llvmbot
Copy link
Member

llvmbot commented Aug 5, 2025

@llvm/pr-subscribers-clang-static-analyzer-1

@llvm/pr-subscribers-clang

Author: Endre Fülöp (gamesh411)

Changes

Binding a value to location can happen when a new value is created or when and existing value is updated. This modification exposes whether the value binding happens at a declaration.
This helps simplify the hacky logic of the BindToImmutable checker.


Patch is 21.91 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/152137.diff

18 Files Affected:

  • (modified) clang/include/clang/StaticAnalyzer/Core/Checker.h (+2-2)
  • (modified) clang/include/clang/StaticAnalyzer/Core/CheckerManager.h (+5-6)
  • (modified) clang/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp (+2-1)
  • (modified) clang/lib/StaticAnalyzer/Checkers/BoolAssignmentChecker.cpp (+3-1)
  • (modified) clang/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp (+4-1)
  • (modified) clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp (+3-2)
  • (modified) clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp (+3-2)
  • (modified) clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp (+3-2)
  • (modified) clang/lib/StaticAnalyzer/Checkers/ObjCSelfInitChecker.cpp (+3-3)
  • (modified) clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp (+1-1)
  • (modified) clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h (+2-1)
  • (modified) clang/lib/StaticAnalyzer/Checkers/StoreToImmutableChecker.cpp (+4-47)
  • (modified) clang/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp (+2-2)
  • (modified) clang/lib/StaticAnalyzer/Checkers/VforkChecker.cpp (+3-2)
  • (modified) clang/lib/StaticAnalyzer/Core/CheckerManager.cpp (+9-7)
  • (modified) clang/lib/StaticAnalyzer/Core/ExprEngine.cpp (+1-1)
  • (modified) clang/unittests/StaticAnalyzer/ExprEngineVisitTest.cpp (+5-3)
  • (modified) clang/unittests/StaticAnalyzer/SValTest.cpp (+2-1)
diff --git a/clang/include/clang/StaticAnalyzer/Core/Checker.h b/clang/include/clang/StaticAnalyzer/Core/Checker.h
index 31cc095c29bfe..fb93fff27e6b3 100644
--- a/clang/include/clang/StaticAnalyzer/Core/Checker.h
+++ b/clang/include/clang/StaticAnalyzer/Core/Checker.h
@@ -209,8 +209,8 @@ class Location {
 class Bind {
   template <typename CHECKER>
   static void _checkBind(void *checker, SVal location, SVal val, const Stmt *S,
-                         CheckerContext &C) {
-    ((const CHECKER *)checker)->checkBind(location, val, S, C);
+                         bool atDeclInit, CheckerContext &C) {
+    ((const CHECKER *)checker)->checkBind(location, val, S, atDeclInit, C);
   }
 
 public:
diff --git a/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h b/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
index c8e6f1265a3db..9441b5f0f14f0 100644
--- a/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
+++ b/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h
@@ -338,10 +338,9 @@ class CheckerManager {
                               ExprEngine &Eng);
 
   /// Run checkers for binding of a value to a location.
-  void runCheckersForBind(ExplodedNodeSet &Dst,
-                          const ExplodedNodeSet &Src,
-                          SVal location, SVal val,
-                          const Stmt *S, ExprEngine &Eng,
+  void runCheckersForBind(ExplodedNodeSet &Dst, const ExplodedNodeSet &Src,
+                          SVal location, SVal val, const Stmt *S,
+                          bool atDeclInit, ExprEngine &Eng,
                           const ProgramPoint &PP);
 
   /// Run checkers after taking a control flow edge.
@@ -499,8 +498,8 @@ class CheckerManager {
   using CheckLocationFunc = CheckerFn<void(SVal location, bool isLoad,
                                            const Stmt *S, CheckerContext &)>;
 
-  using CheckBindFunc =
-      CheckerFn<void(SVal location, SVal val, const Stmt *S, CheckerContext &)>;
+  using CheckBindFunc = CheckerFn<void(SVal location, SVal val, const Stmt *S,
+                                       bool atDeclInit, CheckerContext &)>;
 
   using CheckBlockEntranceFunc =
       CheckerFn<void(const BlockEntrance &, CheckerContext &)>;
diff --git a/clang/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
index 3b3def7ce3324..b8b315c3b829f 100644
--- a/clang/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
@@ -183,7 +183,8 @@ class AnalysisOrderChecker
       llvm::errs() << "NewAllocator\n";
   }
 
-  void checkBind(SVal Loc, SVal Val, const Stmt *S, CheckerContext &C) const {
+  void checkBind(SVal Loc, SVal Val, const Stmt *S, bool atDeclInit,
+                 CheckerContext &C) const {
     if (isCallbackEnabled(C, "Bind"))
       llvm::errs() << "Bind\n";
   }
diff --git a/clang/lib/StaticAnalyzer/Checkers/BoolAssignmentChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/BoolAssignmentChecker.cpp
index 837cbbce8f45f..54de3fafb6bf0 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BoolAssignmentChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BoolAssignmentChecker.cpp
@@ -29,7 +29,8 @@ class BoolAssignmentChecker : public Checker<check::Bind> {
                   bool IsTainted = false) const;
 
 public:
-  void checkBind(SVal Loc, SVal Val, const Stmt *S, CheckerContext &C) const;
+  void checkBind(SVal Loc, SVal Val, const Stmt *S, bool atDeclInit,
+                 CheckerContext &C) const;
 };
 } // end anonymous namespace
 
@@ -55,6 +56,7 @@ static bool isBooleanType(QualType Ty) {
 }
 
 void BoolAssignmentChecker::checkBind(SVal Loc, SVal Val, const Stmt *S,
+                                      bool atDeclInit,
                                       CheckerContext &C) const {
 
   // We are only interested in stores into Booleans.
diff --git a/clang/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp b/clang/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp
index 350db4b1bc2bc..623748cef8273 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp
@@ -175,9 +175,12 @@ class CheckerDocumentation
   /// \param Loc The value of the location (pointer).
   /// \param Val The value which will be stored at the location Loc.
   /// \param S   The bind is performed while processing the statement S.
+  /// \param atDeclInit Whether the bind is performed during declaration
+  ///                  initialization.
   ///
   /// check::Bind
-  void checkBind(SVal Loc, SVal Val, const Stmt *S, CheckerContext &) const {}
+  void checkBind(SVal Loc, SVal Val, const Stmt *S, bool atDeclInit,
+                 CheckerContext &) const {}
 
   /// Called after a CFG edge is taken within a function.
   ///
diff --git a/clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
index 152129ea91fd4..2e0f9d0ce1f1a 100644
--- a/clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
@@ -48,7 +48,8 @@ class DereferenceChecker
 public:
   void checkLocation(SVal location, bool isLoad, const Stmt* S,
                      CheckerContext &C) const;
-  void checkBind(SVal L, SVal V, const Stmt *S, CheckerContext &C) const;
+  void checkBind(SVal L, SVal V, const Stmt *S, bool atDeclInit,
+                 CheckerContext &C) const;
 
   static void AddDerefSource(raw_ostream &os,
                              SmallVectorImpl<SourceRange> &Ranges,
@@ -309,7 +310,7 @@ void DereferenceChecker::checkLocation(SVal l, bool isLoad, const Stmt* S,
 }
 
 void DereferenceChecker::checkBind(SVal L, SVal V, const Stmt *S,
-                                   CheckerContext &C) const {
+                                   bool atDeclInit, CheckerContext &C) const {
   // If we're binding to a reference, check if the value is known to be null.
   if (V.isUndef())
     return;
diff --git a/clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp b/clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
index 7ad54c08d5c71..d57357ea58d5d 100644
--- a/clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp
@@ -150,7 +150,8 @@ class IteratorModeling
   IteratorModeling() = default;
 
   void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
-  void checkBind(SVal Loc, SVal Val, const Stmt *S, CheckerContext &C) const;
+  void checkBind(SVal Loc, SVal Val, const Stmt *S, bool atDeclInit,
+                 CheckerContext &C) const;
   void checkPostStmt(const UnaryOperator *UO, CheckerContext &C) const;
   void checkPostStmt(const BinaryOperator *BO, CheckerContext &C) const;
   void checkPostStmt(const MaterializeTemporaryExpr *MTE,
@@ -234,7 +235,7 @@ void IteratorModeling::checkPostCall(const CallEvent &Call,
 }
 
 void IteratorModeling::checkBind(SVal Loc, SVal Val, const Stmt *S,
-                                 CheckerContext &C) const {
+                                 bool atDeclInit, CheckerContext &C) const {
   auto State = C.getState();
   const auto *Pos = getIteratorPosition(State, Val);
   if (Pos) {
diff --git a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
index 9744d1abf7790..944abfb16aed0 100644
--- a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -97,7 +97,8 @@ class NullabilityChecker
   // libraries.
   bool NoDiagnoseCallsToSystemHeaders = false;
 
-  void checkBind(SVal L, SVal V, const Stmt *S, CheckerContext &C) const;
+  void checkBind(SVal L, SVal V, const Stmt *S, bool atDeclInit,
+                 CheckerContext &C) const;
   void checkPostStmt(const ExplicitCastExpr *CE, CheckerContext &C) const;
   void checkPreStmt(const ReturnStmt *S, CheckerContext &C) const;
   void checkPostObjCMessage(const ObjCMethodCall &M, CheckerContext &C) const;
@@ -1250,7 +1251,7 @@ static bool isARCNilInitializedLocal(CheckerContext &C, const Stmt *S) {
 /// Propagate the nullability information through binds and warn when nullable
 /// pointer or null symbol is assigned to a pointer with a nonnull type.
 void NullabilityChecker::checkBind(SVal L, SVal V, const Stmt *S,
-                                   CheckerContext &C) const {
+                                   bool atDeclInit, CheckerContext &C) const {
   const TypedValueRegion *TVR =
       dyn_cast_or_null<TypedValueRegion>(L.getAsRegion());
   if (!TVR)
diff --git a/clang/lib/StaticAnalyzer/Checkers/ObjCSelfInitChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ObjCSelfInitChecker.cpp
index ace3426387568..17a55e3633998 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ObjCSelfInitChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ObjCSelfInitChecker.cpp
@@ -73,7 +73,8 @@ class ObjCSelfInitChecker : public Checker<  check::PostObjCMessage,
   void checkPreStmt(const ReturnStmt *S, CheckerContext &C) const;
   void checkLocation(SVal location, bool isLoad, const Stmt *S,
                      CheckerContext &C) const;
-  void checkBind(SVal loc, SVal val, const Stmt *S, CheckerContext &C) const;
+  void checkBind(SVal loc, SVal val, const Stmt *S, bool atDeclInit,
+                 CheckerContext &C) const;
 
   void checkPreCall(const CallEvent &CE, CheckerContext &C) const;
   void checkPostCall(const CallEvent &CE, CheckerContext &C) const;
@@ -311,9 +312,8 @@ void ObjCSelfInitChecker::checkLocation(SVal location, bool isLoad,
                 C);
 }
 
-
 void ObjCSelfInitChecker::checkBind(SVal loc, SVal val, const Stmt *S,
-                                    CheckerContext &C) const {
+                                    bool atDeclInit, CheckerContext &C) const {
   // Allow assignment of anything to self. Self is a local variable in the
   // initializer, so it is legal to assign anything to it, like results of
   // static functions/method calls. After self is assigned something we cannot
diff --git a/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
index 62bc3218d9ced..9ae9196a2522b 100644
--- a/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
@@ -1128,7 +1128,7 @@ ExplodedNode * RetainCountChecker::checkReturnWithRetEffect(const ReturnStmt *S,
 //===----------------------------------------------------------------------===//
 
 void RetainCountChecker::checkBind(SVal loc, SVal val, const Stmt *S,
-                                   CheckerContext &C) const {
+                                   bool atDeclInit, CheckerContext &C) const {
   ProgramStateRef state = C.getState();
   const MemRegion *MR = loc.getAsRegion();
 
diff --git a/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h b/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h
index 0e811436605ff..53a14a92be4ec 100644
--- a/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h
+++ b/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h
@@ -290,7 +290,8 @@ class RetainCountChecker
   void printState(raw_ostream &Out, ProgramStateRef State,
                   const char *NL, const char *Sep) const override;
 
-  void checkBind(SVal loc, SVal val, const Stmt *S, CheckerContext &C) const;
+  void checkBind(SVal loc, SVal val, const Stmt *S, bool atDeclInit,
+                 CheckerContext &C) const;
   void checkPostStmt(const BlockExpr *BE, CheckerContext &C) const;
   void checkPostStmt(const CastExpr *CE, CheckerContext &C) const;
 
diff --git a/clang/lib/StaticAnalyzer/Checkers/StoreToImmutableChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StoreToImmutableChecker.cpp
index afad41939cdca..f7100d82c9b55 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StoreToImmutableChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StoreToImmutableChecker.cpp
@@ -26,53 +26,11 @@ class StoreToImmutableChecker : public Checker<check::Bind> {
   const BugType BT{this, "Write to immutable memory", "CERT Environment (ENV)"};
 
 public:
-  void checkBind(SVal Loc, SVal Val, const Stmt *S, CheckerContext &C) const;
+  void checkBind(SVal Loc, SVal Val, const Stmt *S, bool atDeclInit,
+                 CheckerContext &C) const;
 };
 } // end anonymous namespace
 
-static bool isInitializationContext(const Stmt *S, CheckerContext &C) {
-  // Check if this is a DeclStmt (variable declaration)
-  if (isa<DeclStmt>(S))
-    return true;
-
-  // This part is specific for initialization of const lambdas pre-C++17.
-  // Lets look at the AST of the statement:
-  // ```
-  // const auto lambda = [](){};
-  // ```
-  //
-  // The relevant part of the AST for this case prior to C++17 is:
-  // ...
-  // `-DeclStmt
-  //   `-VarDecl
-  //     `-ExprWithCleanups
-  //       `-CXXConstructExpr
-  // ...
-  // In C++17 and later, the AST is different:
-  // ...
-  // `-DeclStmt
-  //   `-VarDecl
-  //     `-ImplicitCastExpr
-  //       `-LambdaExpr
-  //         |-CXXRecordDecl
-  //         `-CXXConstructExpr
-  // ...
-  // And even beside this, the statement `S` that is given to the checkBind
-  // callback is the VarDecl in C++17 and later, and the CXXConstructExpr in
-  // C++14 and before. So in order to support the C++14 we need the following
-  // ugly hack to detect whether this construction is used to initialize a
-  // variable.
-  //
-  // FIXME: This should be eliminated by improving the API of checkBind to
-  // ensure that it consistently passes the `VarDecl` (instead of the
-  // `CXXConstructExpr`) when the constructor call denotes the initialization
-  // of a variable with a lambda, or maybe less preferably, try the more
-  // invasive approach of passing the information forward to the checkers
-  // whether the current bind is an initialization or an assignment.
-  const auto *ConstructExp = dyn_cast<CXXConstructExpr>(S);
-  return ConstructExp && ConstructExp->isElidable();
-}
-
 static bool isEffectivelyConstRegion(const MemRegion *MR, CheckerContext &C) {
   if (isa<GlobalImmutableSpaceRegion>(MR))
     return true;
@@ -128,6 +86,7 @@ getInnermostEnclosingConstDeclRegion(const MemRegion *MR, CheckerContext &C) {
 }
 
 void StoreToImmutableChecker::checkBind(SVal Loc, SVal Val, const Stmt *S,
+                                        bool atDeclInit,
                                         CheckerContext &C) const {
   // We are only interested in stores to memory regions
   const MemRegion *MR = Loc.getAsRegion();
@@ -136,9 +95,7 @@ void StoreToImmutableChecker::checkBind(SVal Loc, SVal Val, const Stmt *S,
 
   // Skip variable declarations and initializations - we only want to catch
   // actual writes
-  // FIXME: If the API of checkBind would allow to distinguish between
-  // initialization and assignment, we could use that instead.
-  if (isInitializationContext(S, C))
+  if (atDeclInit)
     return;
 
   // Check if the region is in the global immutable space
diff --git a/clang/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp
index e98de333f8dd3..43e3ee519c9a6 100644
--- a/clang/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp
@@ -26,13 +26,13 @@ class UndefinedAssignmentChecker
   const BugType BT{this, "Assigned value is uninitialized"};
 
 public:
-  void checkBind(SVal location, SVal val, const Stmt *S,
+  void checkBind(SVal location, SVal val, const Stmt *S, bool atDeclInit,
                  CheckerContext &C) const;
 };
 }
 
 void UndefinedAssignmentChecker::checkBind(SVal location, SVal val,
-                                           const Stmt *StoreE,
+                                           const Stmt *StoreE, bool atDeclInit,
                                            CheckerContext &C) const {
   if (!val.isUndef())
     return;
diff --git a/clang/lib/StaticAnalyzer/Checkers/VforkChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/VforkChecker.cpp
index cb73ac68edd1e..e37f893577df5 100644
--- a/clang/lib/StaticAnalyzer/Checkers/VforkChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/VforkChecker.cpp
@@ -62,7 +62,8 @@ class VforkChecker : public Checker<check::PreCall, check::PostCall,
 
   void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
   void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
-  void checkBind(SVal L, SVal V, const Stmt *S, CheckerContext &C) const;
+  void checkBind(SVal L, SVal V, const Stmt *S, bool atDeclInit,
+                 CheckerContext &C) const;
   void checkPreStmt(const ReturnStmt *RS, CheckerContext &C) const;
 };
 
@@ -188,7 +189,7 @@ void VforkChecker::checkPreCall(const CallEvent &Call,
 }
 
 // Prohibit writes in child process (except for vfork's lhs).
-void VforkChecker::checkBind(SVal L, SVal V, const Stmt *S,
+void VforkChecker::checkBind(SVal L, SVal V, const Stmt *S, bool atDeclInit,
                              CheckerContext &C) const {
   ProgramStateRef State = C.getState();
   if (!isChildProcess(State))
diff --git a/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp b/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
index 0fe677e4ee435..0b133c7f830b7 100644
--- a/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
+++ b/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp
@@ -376,11 +376,13 @@ namespace {
     const Stmt *S;
     ExprEngine &Eng;
     const ProgramPoint &PP;
+    bool AtDeclInit;
 
-    CheckBindContext(const CheckersTy &checkers,
-                     SVal loc, SVal val, const Stmt *s, ExprEngine &eng,
+    CheckBindContext(const CheckersTy &checkers, SVal loc, SVal val,
+                     const Stmt *s, bool atDeclInit, ExprEngine &eng,
                      const ProgramPoint &pp)
-        : Checkers(checkers), Loc(loc), Val(val), S(s), Eng(eng), PP(pp) {}
+        : Checkers(checkers), Loc(loc), Val(val), S(s), Eng(eng), PP(pp),
+          AtDeclInit(atDeclInit) {}
 
     CheckersTy::const_iterator checkers_begin() { return Checkers.begin(); }
     CheckersTy::const_iterator checkers_end() { return Checkers.end(); }
@@ -391,7 +393,7 @@ namespace {
       const ProgramPoint &L = PP.withTag(checkFn.Checker);
       CheckerContext C(Bldr, Eng, Pred, L);
 
-      checkFn(Loc, Val, S, C);
+      checkFn(Loc, Val, S, AtDeclInit, C);
     }
   };
 
@@ -408,10 +410,10 @@ namespace {
 /// Run checkers for binding of a value to a location.
 void CheckerManager::runCheckersForBind(ExplodedNodeSet &Dst,
                                         const ExplodedNodeSet &Src,
-                                        SVal location, SVal val,
-                                        const Stmt *S, ExprEngine &Eng,
+                                        SVal location, SVal val, const Stmt *S,
+                                        bool atDeclInit, ExprEngine &Eng,
                                         const ProgramPoint &PP) {
-  CheckBindContext C(BindCheckers, location, val, S, Eng, PP);
+  CheckBindContext C(BindCheckers, location, val, S, atDeclInit, Eng, PP);
   llvm::TimeTraceScope TimeScope{
       "CheckerManager::runCheckersForBind",
       [&val]() { return getTimeTraceBindMetadata(val); }};
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
index d87484470f8b5..2db25702fc8a9 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -3725,7 +3725,7 @@ void ExprEngine::evalBind(ExplodedNodeSet &Dst, const Stmt *StoreE,
   // Do a previsit of the bind.
   ExplodedNodeSet CheckedSet;
   getCheckerManager().runCheckersForBind(CheckedSet, Pred, location, Val,
-                                         StoreE, *this, *PP);
+                                         StoreE, atDeclInit, *this, *PP);
 
   StmtNodeBuilder Bldr(CheckedSet, Dst, *currBldrCtx);
 
diff --git a/clang/unittests/StaticAnalyzer/ExprEngineVisitTest.cpp b/clang/unittests/StaticAnalyzer/ExprEngineVisitTest.cpp
index 12be2289c3174..8e2b70a1c892f 100644
--- a/clang/unittests/StaticAnalyzer/ExprEngineVisitTest.cpp
+++ b/clang/unittests/StaticAnalyzer/ExprEngineVisitTest.cpp...
[truncated]

@gamesh411
Copy link
Contributor Author

My proposed discussion points about this:
Is the name atDeclInit (OK/meaningful enough) to expose to the API users? (I also thought about: isDecl, isInitialization, belongsToDecl, but none of these seemed great)
Does the extra boolean flag introduce any performance overhead? (I'll try to measure this on some open source projects)

Binding a value to location can happen when a new value is created or
when and existing value is updated. This modification exposes whether
the value binding happens at a declaration.
This helps simplify the hacky logic of the BindToImmutable checker.
@gamesh411 gamesh411 force-pushed the pass-atDeclInit-to-checkBind branch from cab8997 to 5ab97a7 Compare August 5, 2025 13:44
Copy link
Contributor

@steakhal steakhal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use UpperCamelCase style for variable names, including parameter names.

Have you checked if any of the checkBind users used an isa or dyncast as a proxy to determine if its an assignment expression or an initialization?
I suspect there could have been cases where they tried to use this as a proxy, that we could not substitute to this explicit flag instead.

@gamesh411
Copy link
Contributor Author

gamesh411 commented Aug 7, 2025

Have you checked if any of the checkBind users used an isa or dyncast as a proxy to determine if its an assignment expression or an initialization? I suspect there could have been cases where they tried to use this as a proxy, that we could not substitute to this explicit flag instead.

I searched for patterns where other checkers might be using isa/dyn_cast on statement types (like DeclStmt, BinaryOperator) as a proxy to determine if something is an assignment vs initialization. I found only in any of the checkBind implementations. The checkers that do use dyn_cast and dyn_cast are using them for different purposes - such as extracting the right-hand side expression for better error messages, getting the initialization expression from variable declarations, or checking for ARC zero-initialized locals. None of them use statement type checking as a proxy to determine if something should be skipped or handled differently based on whether it's an assignment vs initialisation.

This flag solves the problem of the StoreToImmutable checker in a way that fits the core logic we want to use, and is already information available in the engine itself; this is the biggest pro for its existence.
It could also help new checkers, but that is just speculation.

I did not investigate in detail whether this new flag could be used in the existing checkers, but I suspect that would require some moderate rethinking of their logic, even in the cases where it would be possible. So I would not go that route just for the sake of using this new flag.
The name of UndefinedAssignmentChecker would suggest that it would be useful there, but the current implementation would not benefit from using this flag (or at least I do not see an obvious possibility for meaningful improvement there).

@gamesh411
Copy link
Contributor Author

gamesh411 commented Aug 7, 2025

Does the extra boolean flag introduce any performance overhead? (I'll try to measure this on some open source projects)

Introducing the flag does make the CheckBindContext struct bigger: https://godbolt.org/z/8PT7br4T3
I will have to run measurements to determine whether this is significant in real use cases.

@steakhal
Copy link
Contributor

steakhal commented Aug 7, 2025

Have you checked if any of the checkBind users used an isa or dyncast as a proxy to determine if its an assignment expression or an initialization? I suspect there could have been cases where they tried to use this as a proxy, that we could not substitute to this explicit flag instead.

I searched for patterns where other checkers might be using isa/dyn_cast on statement types (like DeclStmt, BinaryOperator) as a proxy to determine if something is an assignment vs initialization. I found only in any of the checkBind implementations. The checkers that do use dyn_cast and dyn_cast are using them for different purposes - such as extracting the right-hand side expression for better error messages, getting the initialization expression from variable declarations, or checking for ARC zero-initialized locals. None of them use statement type checking as a proxy to determine if something should be skipped or handled differently based on whether it's an assignment vs initialisation.

This flag solves the problem of the StoreToImmutable checker in a way that fits the core logic we want to use, and is already information available in the engine itself; this is the biggest pro for its existence. It could also help new checkers, but that is just speculation.

This is more than enough for me. Thank you.

Does the extra boolean flag introduce any performance overhead? (I'll try to measure this on some open source projects)

Introducing the flag does make the CheckBindContext struct bigger: https://godbolt.org/z/8PT7br4T3 I will have to run measurements to determine whether this is significant in real use cases.

I had saw no performance implications from the getgo. Don't waste your time.

@steakhal
Copy link
Contributor

steakhal commented Aug 7, 2025

There is a relevant test failure in CI:

2025-08-05T13:49:04.5020375Z Failed Tests (1):
2025-08-05T13:49:04.5020697Z   Clang-Unit :: ./AllClangUnitTests/ExprEngineVisitTest/checkLocationAndBind

Run check-clang.

Copy link

github-actions bot commented Aug 7, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Contributor

@steakhal steakhal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, just to avoid confusion. Fix the unittests first; then I'll approve :D
EDIT: And also satisfy clang-format.

calling evalBind was not consistent:
The original issue was that the performTrivialCopy function in
ExprEngineCXX.cpp was always calling evalBind with AtDeclInit = true,
regardless of whether it was handling:
Copy constructors (which should use AtDeclInit = true for
initialization)
Assignment operators (which should use AtDeclInit = false for
assignment)
@gamesh411
Copy link
Contributor Author

gamesh411 commented Aug 7, 2025

I have stumbled upon a possible inconsistency in the way performTrivialCopy in ExprEngineCXX calls evalBind (by extending the test case to see this new flag as well).
With the above proposed fix, I think it may no longer be NFC technically. Please feel free to remove the tag if you see fit.
Or this fixing of the ExprEngine could become a separate commit, what do you think?

@steakhal
Copy link
Contributor

steakhal commented Aug 7, 2025

With the above proposed fix, I think it may no longer be NFC technically. Please feel free to remove the tag if you see fit.
Or this fixing of the ExprEngine could become a separate commit, what do you think?

NFCi is fine.

@steakhal steakhal changed the title [clang][analyzer][NFCi] Pass if bind is to a Decl or not to checkBind [analyzer][NFCi] Pass if bind is to a Decl or not to checkBind Aug 8, 2025
@steakhal steakhal merged commit 4f2ed92 into llvm:main Aug 8, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:static analyzer clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants